Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes to make mutmut3 work with inline-snapshot #338

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

15r10nk
Copy link
Contributor

@15r10nk 15r10nk commented Oct 23, 2024

this is currently all work in progress ...

it is important that the actual inline-snapshot package is NOT part of the venv.

steps to create the venv:

... inside inline-snapshot repo using the mutmut3 branch

python -m venv mutmut_venv
source mutmut_venv/bin/activate.sh
python -m pip install ../mutmut # my local mutmut fork
mutmut run # which will fail but copy the code
python -m pip install -r requirements.txt
mutmut run # which should work now

@@ -683,6 +687,8 @@ def new_tests(self):
class PytestRunner(TestRunner):
def execute_pytest(self, params, **kwargs):
import pytest
params+=["--rootdir=.","--inline-snapshot=disable"]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This seems a bit weird. Do we need some config option in mutmut for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, I think it should not be required and it could also be a problem in inline-snapshot. But this is currently useful to make things work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --rootdir was important. I had several problems to convince mutmut to test the code under mutants/

output_catcher.dump_output()
print(f'failed to collect stats. runner returned {collect_stats_exit_code}')
exit(1)
collect_stats_exit_code = runner.run_stats(tests=tests)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use outputcatcher.stop() to stop it from swallowing output. This is a better method for debugging I find.

Although, the real solution is to add a flag or config to turn it off globally I guess...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the Tip

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 23, 2024

@boxed this is where I'm currently:

❯ mutmut run
generating mutants
    done in 9ms
⠼ collecting stats
..s..,,.,,,,.,,,,.,,,,..,,.,,,.,,.,,.,,..,,,,..,,,,..s,,.,,.,,.s.,,,,....,,,..,,,,.,,,,..,,..,,,,..s,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,...,,.,,.s.,,.....s,,.,,,.,,,,.,,.,,..,,,,.,,,,.,,,,.,,,,.,,,,.,,,..s.,,, [ 18%]
,.,,,,.,,,,..,,,,.....s,,..,,.,,.s,,.s.,,,..,,,,.,,,,.,,,,....,,.,,.,,...,,,,.,,,,.,,,,.,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,.,,.,,.,,....s.,,,,.,,,,.,,,,.,,,.,,...,,.,,..,,,,...,,,,.., [ 36%]
,,,.,,,,.,,,,.,,,,.,,.,,...s.s,,..,,,,......,,,,.,,,,..s..,,.,,.,,.,,.,,.,,,.,,,.......,,,,..,,,..,,,,.,,.,,.,,,.,,,,.,,,,..,,,.,,,.,,,.,,,,.,,,,...,,,,.,,,,.,,,,.,,,,...,,.,,.,,.,,.,,.s,,.s,,,,.s,,.s,,, [ 55%]
,.......,,,,.s,,,,.,,.,,.,,.,,.,,.,,.,,.s,,,,.,,.,,...,,.,,,,.,,,,.,,,,.,,,,.,,,,....,,...,,..s.,,,,..,,.,,.,,..,,.....s.,,,,..,,,,.,,,,...,,,,...,,,...s,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,, [ 75%]
,,.s.,,,...s,,.,,,,.,,,,.s,,,,.s..,,,,.,,,,...,,,,.,,,,.,,.....,,.,,.,,.,,.,,...s.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,,.,,,..,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,.,,.,,,,...,,.,,,,.s.,,.,,.,,,,,,,,,,,,,,,,,,,,,,,, [ 89%]
,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,.,,.,,...,,,..,,,.,,,.,,.,,.,,.,,., [ 93%]
,,.,,,..........,,,,.,,,,...,,,,.,,,,.,,,,.,,,,.,,,,.,,...,,.,,...                                                                                                                                          [100%]
352 passed, 29 skipped, 903 subtests passed in 69.02s (0:01:09)
    done
⠸ running clean tests
    done

    done
Running mutation testing
⠴ 1821/1821  🎉 0 🫥 1821  ⏰ 0  🤔 0  🙁 0  🔇 0
0.00 mutations/second

I have no Idea why it does not test the mutations.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 23, 2024

I fixed some src-layout related issues (I dont know if I fixed them in the right way, but It works for me for now).
I can test the mutations now but one mutation starts to use 100% of my system memory and mutmut run stops there and is not able to test all mutations.

... something for another day.

@boxed maybe you can use the fixes here as a base for some real fixes. I don't understand your whole code base and don't know where these things should be fixed.

@boxed
Copy link
Owner

boxed commented Oct 24, 2024

Cool! Yea that's great!

Timeout for mutants that introduce infinite loops isn't implemented yet. I just disabled break->continue mutation and similar to avoid the scenario as far as possible without timeout support. But I suspected that this won't work 100% and unfortunately this happened sooner than I expected 🤣

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 24, 2024

nice, I got the memory limit working.
Testing the mutations got also much faster, which is very cool. I always wanted to run mutmut in ci and add a "mutant kill count" badge to my readme, I think this is definitely possible now 😄.

The last open problem is propably my own pytest plugin which comes with inline-snapshot.
I have the feeling that it interacts somehow with the test runs inside mutmut. I will have a lock at it later.

@boxed
Copy link
Owner

boxed commented Oct 24, 2024

Memory limit is an interesting idea. You could set it at like 20% over the current memory, as the forked process should have loaded 100% of what is needed from the parent clean test run. That could probably ship in mutmut as is.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 24, 2024

The limit is per process. I experienced that the memory usage got much higher than the limit because multiple tests where using a lot of memory at the same time.
The limit should not be bigger than TotalMemory/number of parallel tests.

You could set it at like 20% over the current memory, as the forked process should have loaded 100% of what is needed from the parent clean test run.

But there is some memory which gets freed, or? and what is if the application has only one test. 20% memory would not be enough in this case

@boxed
Copy link
Owner

boxed commented Oct 24, 2024

Yea you're right. It should be x% over of the peak memory usage during the clean run, which is probably hard to capture correctly.

I'm used to working with systems where the import graph dominates the memory usage, but you're right in that this is not generally true.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 24, 2024

this is a bit off topic but you might like https://github.com/15r10nk/lazy-imports-lite if you have a lot of imports 😃

@boxed
Copy link
Owner

boxed commented Oct 24, 2024

Ooh. That is some dark magic 🤣 Cool!

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 24, 2024

Do you have any idea how you could limit the time which is used to test a mutant? I think my tests getting slower because some of the parallel mutant tests are running forever.

I was also never able to finish the mutant tests.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 24, 2024

I get so far and the progress stops then.

⠸ 1817/1829 🎉 1498 🫥 209 ⏰ 0 🤔 0 🙁 110 🔇 0

@boxed
Copy link
Owner

boxed commented Oct 25, 2024

I think the idea forward is to implement something similar to how mutmut2 worked: time the full test run, and forcibly kill all workers that have run longer than the full time * 1.5 or something like that. There was some rule to have a minimum time added before the timeout value was implemented to handle really small test suites, but that was mostly it I think.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 25, 2024

but the problem is that the whole thing can block if you limit the number of parallel processes to 15 maybe and 15 process decide to never terminate. Mutmut will not be able to handle the remaining mutations any more.

What about this:

  • you know which tests you are running and I think you also know how long they run normally.
  • you can take this time (times 5 maybe) and use this as a limit for the process runtime.

@boxed
Copy link
Owner

boxed commented Oct 25, 2024

but the problem is that the whole thing can block if you limit the number of parallel processes to 15 maybe and 15 process decide to never terminate

That's what I was talking about yea. The master process that forks off children will never run mutation. It should store start time for each PID, and if a child has lived longer than the timeout it sends kill -9 to it and marks it as timed out in the result.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 25, 2024

I have fixed some src-layout related issues in my branch but I have still problems with mutmut browse probably caused by this changes.

image
(I added the traceback there)

I have the assumption that this caused by some general bug how some keys are generated or so.
Could you take a look at it If you have time?

@boxed
Copy link
Owner

boxed commented Oct 25, 2024

Ah I think you need to delete the mutants folder and try again. The name mangling was changed and you have the old stuff on disk.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 25, 2024

I delete it everytime before I run mutmut run. I tried to fix some src. related issues in this branch but I thing I fixed the problem always in the wrong way.

@boxed
Copy link
Owner

boxed commented Oct 25, 2024

I pushed a few of fixes on main.

@boxed
Copy link
Owner

boxed commented Oct 25, 2024

resource.RLIMIT_CPU looks very interesting as a way to implement timeout totally in the worker process.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 25, 2024

mutmut browse works now also for me. I added also some diff syntax highlight to the browser.

@boxed
Copy link
Owner

boxed commented Oct 25, 2024

Nice. I looked into syntax highlighting, but I failed at finding it heh

@boxed
Copy link
Owner

boxed commented Oct 25, 2024

It's weirdly recollecting stats every single time. So something is wrong there.

In any case, I got it to run through the entire test suite (if I rm mutants/ before)!

⠦ 1829/1829  🎉 1473 🫥 209  ⏰ 0  🤔 0  🙁 147  🔇 0

all changes for that are on main.

@15r10nk 15r10nk marked this pull request as ready for review October 25, 2024 21:00
@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 25, 2024

I squashed everything into 2 commits. The fixes and the diff-syntax support.
Let me know what you think.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 25, 2024

damn, mutmut is not deterministic (at least not for inline-snapshot)

I run rm mutants -r ; mutmut run twice and got the following results

⠋ 1829/1829 🎉 1355 🫥 209 ⏰ 0 🤔 0 🙁 265 🔇 0
⠋ 1829/1829 🎉 1322 🫥 209 ⏰ 0 🤔 0 🙁 298 🔇 0

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

Well that's interesting to say the least!

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

I get this a lot:

  File "/Users/boxed/Projects/mutmut/mutmut/__main__.py", line 138, in limit_memory
   resource.setrlimit(resource.RLIMIT_AS, (maxsize, hard))
ValueError: current limit exceeds maximum limit

Maybe the memory limit idea isn't good? The resource CPU time limit should cover this problem better no? Because if the process starts allocating a lot and that starts to swap and stuff it will hit the CPU limit pretty fast anyway. Or am I not understanding the issue?

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

I cherry picked the syntax highlighting fix into main.

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

I released 3.2.0. This version runs through inline-snapshot/mutmut3:

⠹ 1829/1829 🎉 1461 🫥 209 ⏰ 0 🤔 0 🙁 159 🔇 0

It does weirdly print a bunch of output for some failed mutants, which I don't understand.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 26, 2024

The reason for the output is that inline-snapshot comes with a pytest plugin which gets also mutated during test.
What you see are exceptions during the pytest run. They are not catched like normal test failures because they happen outside the tests.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 26, 2024

Some of my fixes are still necessary for inline-snapshot.

This is what I get with the current main:

  File "/home/frank/projects/inline-snapshot/mutants/src/inline_snapshot/__init__.py", line 46, in <module>
    from ._code_repr import customize_repr
  File "/home/frank/projects/inline-snapshot/mutants/src/inline_snapshot/_code_repr.py", line 562, in <module>
    def _(value: Enum):
  File "/home/frank/projects/inline-snapshot/mutants/src/inline_snapshot/_code_repr.py", line 410, in customize_repr
    result = _mutmut_trampoline(x_customize_repr__mutmut_orig, x_customize_repr__mutmut_mutants, *args, **kwargs)
  File "/home/frank/projects/inline-snapshot/mutants/src/inline_snapshot/_code_repr.py", line 12, in _mutmut_trampoli
ne
    record_trampoline_hit(orig.__module__ + '.' + orig.__name__)
  File "/home/frank/projects/mutmut/mutmut/__main__.py", line 136, in record_trampoline_hit
    if mutmut.config.max_stack_depth != -1:
AttributeError: 'NoneType' object has no attribute 'max_stack_depth'

inline-snapshot runs some tests in a subprocess where mutmut is not initialized.

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

Ah. Crap. The reason for that is that I have that setting in my conf locally. My bad.

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

No, that's not it. I misremembered. Then I don't understand.

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

inline-snapshot runs some tests in a subprocess where mutmut is not initialized

Well this crash may be a hitbug then. If we make config load automatically, or if we ignore it, then we will have a silent error in the trampoline hit code, so mutmut won't get a trampoline hit registered. That's worse.

I don't know what the solution to that is. I don't want cross-process communication for this if it can be avoided.

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 26, 2024

The mutliprocess support of coverage.py uses different coverage files per process.

Could mutmut use a seperate file mutmut-stats-$pid.json for each process and combine these files after the stats run is finished?

The location of these files should be derived from an env variable because the process can have a different working directory.

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

I would be worried about pid based filenames about reuse. Maybe an env variable could give a full path to the file and the master process decides the name to avoid collisions.

@@ -957,36 +957,59 @@ def should_ignore_for_mutation(self, path):
return False


@lru_cache()
def read_config():
def config_reader():
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean it ONLY reads from pyproject.toml and now ignores setup.cfg? That's no good imo. If mutmut should force pyproject.toml it should at the very least error out if there is a mutmut section in setup.cfg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the config reader is your old s() function. It returns the old setup.cfg reader if there is no pyproject.toml with an [tool.mutmut] key.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I cherry-picked this fix into main

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also cherry-pick my fix for the debug option.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@15r10nk
Copy link
Contributor Author

15r10nk commented Oct 26, 2024

I would be worried about pid based filenames about reuse

There can never be two subprocesses with the same pid at the same time.
But you are right that a process should probably read an existing file and add new data to it instead of overwriting an existing one.

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

Ah. True. If it just reads before writing then pid reuse would be totally fine. Maybe even better.

@boxed
Copy link
Owner

boxed commented Oct 26, 2024

I added a debug config option to mutmut on main that will turn off all output swallowing, and passes -vv to pytest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants